Add logging support for init containers in KubernetesPodOperator (#42498)#43853
Conversation
mrk-andreev
requested review from
hussein-awala and
jedcunningham
as code owners
boring-cyborg
Bot
added
area:providers
provider:cncf-kubernetes
|
@dstandish @jedcunningham @jscheffl @hussein-awala -> that one was submitted during the Man's Hackathon - I am not sure if I am competent enough to review the details, but it looks good at a first glance, I'd need someone more k8s-api-involved to review it :) |
jscheffl
left a comment
jscheffl
left a comment
There was a problem hiding this comment.
I also only feel 50% competent, added some feedback from just code reading.
I am not an K8s expert, can init containers run in parallel? Or are they executed serial?
How about if init containers run for a longer time? Would it be interesting to stream logs while they are running to see progress? And if they run in parallel would it mix logs?
mrk-andreev
force-pushed
the
issue-42498
branch
from
752e733 to
d23bd7b
Compare
Each init container runs sequentially, with each one waiting for the previous container to complete. The main containers wait until all init containers are ready before starting. Ref: https://kubernetes.io/docs/concepts/workloads/pods/init-containers/#understanding-init-containers
|
mrk-andreev
force-pushed
the
issue-42498
branch
2 times, most recently
from
d58a784 to
c77bc4d
Compare
mrk-andreev
changed the title
mrk-andreev
force-pushed
the
issue-42498
branch
2 times, most recently
from
c0e0a1d to
bbb224d
Compare
hussein-awala
left a comment
hussein-awala
left a comment
There was a problem hiding this comment.
Overall it looks good, just need to check if the pod was started before you start reading the logs from init containers, the rest is nit.
mrk-andreev
force-pushed
the
issue-42498
branch
4 times, most recently
from
15527a3 to
9baf12d
Compare
mrk-andreev
force-pushed
the
issue-42498
branch
2 times, most recently
from
fb62e27 to
e710eae
Compare
|
Hi @jscheffl, @hussein-awala, @dstandish, @potiuk, @jedcunningham Does this PR need any additional changes? Are there any blockers we should address? Let me know how I can help to move it forward! |
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
LGTM. But I think some questions/answers need confirmation from @hussein-awala and @dstandish
|
Can you rebase in the meantime @mrk-andreev ? |
mrk-andreev
force-pushed
the
issue-42498
branch
from
e710eae to
5221d13
Compare
eladkal
left a comment
eladkal
left a comment
There was a problem hiding this comment.
LGTM
If no further comments I will merge this PR for next provider wave
Let's. I think it looks good. |
…che#42498) This change adds an option to print logs for init containers. The get_init_containers_logs and init_container_logs functions allow displaying the logs of init containers. Fixes: apache#42498
|
I rebased it - and let's give @dstandish and @hussein-awala last chance to comment. |
…che#42498) (apache#43853) This change adds an option to print logs for init containers. The get_init_containers_logs and init_container_logs functions allow displaying the logs of init containers. Fixes: apache#42498
6 participants
This change adds an option to print logs for init containers. The init_container_logs argument enables the display of logs specifically for spec.initContainers (not spec.containers).
Fixes: #42498